Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit format parameter to docvalue_fields requests #22771

Merged
merged 7 commits into from
Sep 7, 2018

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Sep 6, 2018

Fixes #22484

Elasticsearch 6.4 added an optional format parameter for docvalue_fields.
In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in millis, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users. The top_hits agg was the only other place
in the code where I saw us using docvalue_fields so I updated that as well.

Fixes elastic#22484

Elasticsearch 6.4 added an optional `format` parameter for doc_value
fields. In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in millis, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users.
Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does remove the deprecation warnings for me, and everything appears to work fine. I'm curious though, what's the different between "the same values we see in scripted fields" and the new behavior with use_field_mapping?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Sep 6, 2018

what's the different between "the same values we see in scripted fields" and the new behavior with use_field_mapping?

I don't know the answer to this across every single field type. For dates at least, it means the value returned from docvalue_fields will be based on the format provided in the mappings instead of always returning millis. As far as I can tell from the docs, the format mapping param only applies to date fields, so maybe only date fields are affected by use_field_mapping. Is that right @clintongormley ?

@Bargs
Copy link
Contributor Author

Bargs commented Sep 6, 2018

might be a flaky test, I seem to remember that one failing on other PRs. Gonna re-run on CI while I'm looking at it locally

jenins, test this

@Bargs
Copy link
Contributor Author

Bargs commented Sep 6, 2018

Ok, that's a legit test failure. I was positive we were getting millis back from docvalue_fields but it looks like that's not actually the case. Currently in master and 6.5 they're coming back as ISO strings, so I switched the format param in this PR to strict_date_optional_time. I am not totally sure if this is 100% equivalent to the default in 6.x. Maybe @clintongormley can confirm that as well, or at least point me to an ES dev who can.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jimczi
Copy link

jimczi commented Sep 7, 2018

@Bargs usinguse_field_mapping on a date field will pick the first format defined in the mapping for that field. If you want to format date as number of millis since epoch you have to use "format": "epoch_millis". This is the reason why we need a deprecation warning in 6x; use_field_mapping (which is the default on 7) is not the same as not setting a format.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I can't think of any other places to update off-hand, and this does result in those deprecation headers no longer being returned.

@Bargs Bargs merged commit 4447523 into elastic:master Sep 7, 2018
Bargs added a commit to Bargs/kibana that referenced this pull request Sep 7, 2018
…2771)

Fixes elastic#22484

Elasticsearch 6.4 added an optional `format` parameter for doc_value
fields. In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in ISO format, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users.
Bargs added a commit to Bargs/kibana that referenced this pull request Sep 7, 2018
…2771)

Fixes elastic#22484

Elasticsearch 6.4 added an optional `format` parameter for doc_value
fields. In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in ISO format, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users.
Bargs added a commit that referenced this pull request Sep 7, 2018
…22837)

Fixes #22484

Elasticsearch 6.4 added an optional `format` parameter for doc_value
fields. In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in ISO format, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users.
Bargs added a commit that referenced this pull request Sep 7, 2018
…22838)

Fixes #22484

Elasticsearch 6.4 added an optional `format` parameter for doc_value
fields. In 6.x if the param is not included it defaults to returning
the same values we see in scripted fields. In 7.0 this is changing to
use the mapping configured format by default. In kibana we want our date
values in ISO format, so this PR future proofs us for 7.0. It also eliminates
deprecation warnings ES is returning due to the missing param, which is
currently spamming some users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants